Skip to content

gh-1243: Remove unused healpix/healpy wrappers - #1245

Open
paddyroddy wants to merge 48 commits into
mainfrom
paddy/issue-1243
Open

paddyroddy wants to merge 48 commits into
mainfrom
paddy/issue-1243

Conversation

@paddyroddy

@paddyroddy paddyroddy commented Oct 7, 2026 •

Copy link
Copy Markdown
Member

Description

Every time we call to healpy or healpix we are using NumPy data and therefore lose out on autodifferentiability etc. This PR tries to reduce the usage, and is split into a few parts.

  1. ang2vec was no longer being used anyway, this should have been removed in the deprecated functions.
  2. almxfl has been replaced with glass.harmonics.multalm, this should have been done before.
  3. get_nside has been replaced by glass.harmonics.npix2nside.
  4. pixwin and pol have been removed from glass.harmonics.inverse_transform/glass.healpix._alm2map. These weren't having any affect when they were used in the code, and doesn't make sense in an S2FFT context.
  5. pol has been removed from glass.harmonics.transform/glass.healpix._map2alm for similar reasons. use_pixel_weights has been set to True always as it was the only time it was used within GLASS.

@connoraird and I discussed this on a call whether the arguments are safe to remove:

  • pixwin in _alm2map_ was always False as was the default
  • use_pixel_weights in _map2alm was always True, so set as default
  • pol in both _alm2map_alm and _map2alm wasn't doing anything as the input was always alm. As explained in the healpy docs.

Closes: #1243

Changelog entry

Added: Some new feature
Changed: glass.harmonics.transform now always has use_pixel_weights=True
Changed: glass.healpix.alm2map has now been made private glass.healpix._alm2map
Changed: glass.healpix.map2alm has now been made private glass.healpix._map2alm
Removed: pixwin and pol from glass.healpix._alm2map
Removed: pol from glass.healpix._map2alm
Removed: the glass.healpix.almxfl, glass.healpix.ang2vec and glass.healpix.get_nside wrappers.

Checks

  • Is your code passing linting?
  • Is your code passing tests?
  • Have you added additional tests (if required)?
  • Have you modified/extended the documentation (if required)?
  • Have you added a one-liner changelog entry above (if required)?

@paddyroddy paddyroddy self-assigned this Oct 7, 2026
@paddyroddy
paddyroddy added this pull request to stack #1246 October 7, 2026 08:03
@paddyroddy
paddyroddy removed this pull request from stack #1246 October 7, 2026 08:56

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Two offline pixel-weight tests no longer exercise pixel-weight loading because they omit use_pixel_weights=True.

Review effort: Balanced
Findings: None

Resolved since last review (2)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Enable pixel weights to exercise offline fixture data

tests/​core/​test_healpix.py:122

This test no longer exercises the pulled pixel-weight data: map2alm still defaults use_pixel_weights to False, so Healpy does not read weights from the fixture's HEALPY_DATAPATH. Keep use_pixel_weights=True here while removing only the obsolete pol argument so the offline-data path remains covered.

This issue also appears on line 138 of the same file.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Unconditional pixel weights break transforms for supported HEALPix resolutions lacking weight files.

Review effort: Balanced
Findings: 1 High severity

Open (1)

Comment thread glass/harmonics.py Outdated
"""
return hp.map2alm(maps, lmax=lmax, pol=pol, use_pixel_weights=use_pixel_weights)
# the only time GLASS used map2alm was with pixel weights
return hp.map2alm(maps, lmax=lmax, use_pixel_weights=True)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks — I’ve kept glass.healpix.map2alm unchanged. The use_pixel_weights=True change is only in glass.harmonics.transform, where it’s part of the S2FFT-backed path rather than the HEALPix compatibility API.

@paddyroddy
paddyroddy requested a review from connoraird October 7, 2026 14:02
@paddyroddy paddyroddy added the needs-2-reviewers Could be considered "controversial" so worth a second pair of eyes label Oct 7, 2026
@paddyroddy
paddyroddy requested a review from ntessore October 7, 2026 15:41
Base automatically changed from paddy/issue-794 to main October 7, 2026 16:07

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Unconditional pixel weights break transforms at unsupported resolutions, including the NSIDE 1 test case.

2 open findings

🧠 Review effort: Balanced


Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.

Comment thread glass/harmonics.py
"""
return hp.map2alm(maps, lmax=lmax, pol=pol, use_pixel_weights=use_pixel_weights)
# the only time GLASS used _map2alm was with pixel weights
return _map2alm(maps, lmax=lmax, use_pixel_weights=True)
@paddyroddy
paddyroddy added this pull request to stack #1260 October 15, 2026 14:13
@paddyroddy
paddyroddy removed this pull request from stack #1260 October 15, 2026 14:14
@paddyroddy
paddyroddy added this pull request to stack #1262 October 15, 2026 14:54
paddyroddy added a commit that referenced this pull request Oct 15, 2026
@paddyroddy
paddyroddy force-pushed the paddy/issue-1243 branch 2 times, most recently from 8810ad6 to 90d8de0 Compare October 15, 2026 15:06
@paddyroddy
paddyroddy removed this pull request from stack #1262 October 15, 2026 15:14

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api An (incompatible) API change array-api Work is related to the Array API enhancement New feature or request maintenance Maintenance: refactoring, typos, etc. needs-2-reviewers Could be considered "controversial" so worth a second pair of eyes spherical-transform Changed related to spherical transforms, i.e. HEALPix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Reduce calls to healpy when unncessary

3 participants